Conversation
FrenchGithubUser
force-pushed
the
tt/tim-config-flag
branch
from
b8d6cdb to
e8de83f
Compare
There was a problem hiding this comment.
Pull request overview
Adds a new tim_version configuration flag to the Synapse homeserver config, and wires tests to allow selecting the TIM version globally (via env var) or per-test.
Changes:
- Introduces
TimConfigwith validation of allowed TIM versions. - Registers the new config section in
HomeServerConfig. - Adds tests and test utilities support for configuring
tim_version.
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
tests/utils.py |
Adds TIM_VERSION_FOR_TESTS env-based default and injects tim_version into generated test config. |
tests/config/test_tim.py |
New tests validating tim_version parsing and error handling. |
synapse/config/tim.py |
New config section implementing tim_version parsing/validation. |
synapse/config/homeserver.py |
Registers TimConfig so it’s loaded into HomeServerConfig. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| def read_config(self, config: JsonDict, **kwargs: Any) -> None: | ||
| tim_version = config.get("tim_version", "1.1") | ||
|
|
||
| if tim_version not in VALID_TIM_VERSIONS: | ||
| raise ConfigError( | ||
| f"tim_version must be one of {', '.join(VALID_TIM_VERSIONS)}, " | ||
| f"got {tim_version!r}", | ||
| ("tim_version",), | ||
| ) |
There was a problem hiding this comment.
tim_version values in YAML may be parsed as numbers if unquoted (e.g. tim_version: 1.2). Currently those will always be rejected because you're comparing against string literals. Consider coercing tim_version to str (similar to how default_room_version is handled) before validation, or at least raise a clearer error telling users to quote the value.
| def test_default_tim_version(self) -> None: | ||
| """tim_version defaults to '1.1' when not set.""" | ||
| config = self._parse_config({}) | ||
| self.assertEqual(config.tim.tim_version, "1.1") | ||
|
|
There was a problem hiding this comment.
This test doesn't currently exercise the "not set" default: default_config() now always includes tim_version (from TIM_VERSION_FOR_TESTS), so _parse_config({}) is still explicitly setting it. Consider removing/popping tim_version from the base config dict for this test so it actually verifies the TimConfig default and doesn't fail when SYNAPSE_TIM_VERSION is set for the test run.
| def test_invalid_tim_version_numeric(self) -> None: | ||
| """A numeric (non-string) tim_version should raise ConfigError.""" | ||
| with self.assertRaises(ConfigError): | ||
| self._parse_config({"tim_version": 1.1}) |
There was a problem hiding this comment.
If tim_version is intended to be configured via YAML, users may naturally write tim_version: 1.1 / 1.2 (unquoted), which YAML parses as a number. Right now the test enforces that numeric values error, but that would make common config files fail. Consider either accepting numeric values by coercing to str, or updating the error message + tests to explicitly require quoting.
| def test_invalid_tim_version_numeric(self) -> None: | |
| """A numeric (non-string) tim_version should raise ConfigError.""" | |
| with self.assertRaises(ConfigError): | |
| self._parse_config({"tim_version": 1.1}) | |
| def test_tim_version_numeric_coerced(self) -> None: | |
| """A numeric tim_version is accepted by coercing it to a string.""" | |
| config = self._parse_config({"tim_version": 1.1}) | |
| self.assertEqual(config.tim.tim_version, "1.1") |
SYN-14
adds a config flag to set the tim version (enabling the corresponding features once implemented).
Regarding tests, this flag can be selected gloablly in
tests/utils.pywith the variableTIM_VERSION_FOR_TESTSor for single tests with@override_config({"tim_version": "1.2"})for example